PgSQL native backend protocol: replace libpq on the data path (connect/auth/TLS, simple query, COPY, extended query via stmt pipeline, Describe cache) - #5882
Open
renecannao wants to merge 98 commits into
Conversation
…ing + unit tests Add PgSQL_Backend_Msg_Framer: a pure wire-message framer for the native PostgreSQL backend protocol. It accepts fed bytes (possibly partial) and yields complete messages (type byte + 4-byte big-endian length-prefixed body), signaling FRAME_NEED_MORE on incomplete trailing bytes and FRAME_ERROR on a malformed length. Header stays light (cstdint/cstddef only). Destructor frees the realloc'd buffer to avoid a leak on long-lived connections. Wire it into libproxysql.a via _OBJ_CXX in lib/Makefile. Also fix a pre-existing duplicate vec.o on the unit-test link line: the test/tap/tests/unit/Makefile appended SQLITE3_LDIR/vec.o to STATIC_LIBS in two separate PROXYSQL40 blocks, producing ~98 duplicate-symbol errors that broke every unit test under PROXYSQL40. Keep the single append after the autodetection block.
…hecks, noncopyable)
…proof-correctness test
…nc_connect assert) + timeout teardown [Task 1.6a]
… verification) [Task 1.6b]
…nding Docker) [Task 1.8]
…hrough results) [Task 1.6c / Phase 2 core]
The native simple-query path appended a NUL to query.length bytes, but the client-query callers (async_query with pgsql_real_query.QuerySize) pass a length that already includes the trailing NUL, producing a malformed double-NUL Query body. PostgreSQL rejects it with 08P01 'invalid message format', breaking the backend connection. Normalize to the SQL up to the first NUL (bounded by query.length) plus a single terminator, matching PQsendQuery semantics. The strlen()-based callers (async_send_simple_command/init_connect) are unaffected.
…nnection is_connection_in_reusable_state() called PQtransactionStatus(pgsql_conn) directly; in native mode pgsql_conn is NULL so libpq returns PQTRANS_UNKNOWN, making the session treat a normal backend query error (ErrorResponse + ReadyForQuery, the connection is still idle/reusable) as a broken connection and retry instead of forwarding the error (with its SQLSTATE) to the client. Derive the transaction status from the natively-tracked ReadyForQuery byte in native mode.
…nd-point,, when cbind is set
… both offered and TLS
Contributor
Author
Hardening round: cancellation, sanitizer pass, and the promised benchmarkNative query cancellation implemented ( ASAN pass over the whole native suite (dedicated worktree, full sanitizer build): 10 TAP tests + 12 unit binaries green under ASAN; the branch's manual-memory surfaces (portal registry, raw captures, describe cache, framer, builders) produced zero sanitizer records — except one real find: a use-after-free between named-portal teardown and the event logger ( Benchmarking found a third bug before producing numbers: native mode registered explicit transactions twice (per-ReadyForQuery handler + shared epilogue), tripping a per-transaction "no transaction in progress" warning — 3.2M log lines / ~900MB during a single pgbench run. Fixed by removing the duplicate call, with a reviewer-verified invariant proof that the epilogue covers every native completion path ( Benchmark (release build, host-run proxysql, dedicated postgres:16 backend, 54×60s interleaved runs, 3 passes/cell; native-vs-libpq, median tps / proxy CPU):
The read-path gains at equal-or-lower proxy CPU are consistent with the double-copy elimination this PR exists for. The tpcb@c32 cell (fully write-saturated through one backend) is within noise across passes; flagged honestly rather than averaged away. Full methodology + limitations in the branch session reports ( Filed along the way: #5896 (connect-path debug assert on unresolvable host), #5897 (pre-existing native SET-tracking gaps), #5904/#5905/#5906 (concurrent soak, TLS corpus, fake-server robustness harness — the queued testing follow-ups). Agent-facing infra hazard catalog added to Current test matrix at HEAD |
…backend-protocol # Conflicts: # lib/PgSQL_Session.cpp
…mits
The proxy_debug() macro is gated on the admin-debug master switch
(GloVars.global.gdbg, include/proxysql_debug.h): unless admin-debug='true',
every proxy_debug() call is a runtime no-op and no MOD# line is ever written
to the foreground/teed proxysql.log.
The docker-pgsql16-single infra never provisioned admin-debug — unlike every
MySQL infra, whose docker-proxy-post.bash applies conf/proxysql/infra-config.sql
(SET admin-debug='true'; admin-debug_output=2; debug_levels verbosity=7). So
debug-level markers scraped by pgsql-native_prepared-t P25/P26 ("Describe
served from metadata cache", emitted at proxy_debug(PROXY_DEBUG_MYSQL_COM,5))
could never appear -> cache_hits_2nd_mode=0.
Mirror the MySQL convention in config.sql (applied by docker-proxy-post.bash):
enable admin-debug, keep debug_output=2 (debug DB only, no stderr flood), and
set module verbosity=7 (except pkt_array/net). Tests raise debug_output to 3
for their scrape phase via DebugLogScope.
Not a v3.0 code regression: all debug-propagation code (debug.cpp,
proxysql_debug.h, main.cpp gdbg default, ProxySQL_Admin.cpp gdbg/set_variable)
is byte-identical across 181e87c..89ed5b6; the FlushVariableStats admin
refactor is purely additive and debug output works end-to-end once admin-debug
is enabled. This is a latent infra provisioning gap this infra always had.
|
plisandro
pushed a commit
to plisandro/proxysql
that referenced
this pull request
Aug 10, 2026
- New infras use dbdeployer (infra-dbdeployer-pgsql17-repl), matching the existing infra-dbdeployer-* convention; first dbdeployer PG infra. - Frame the initial phase as discovery (failure inventory, xfail catalogue), no expectation of 100% success; SP-2 CI is reporting-oriented. - Add backend-protocol mode (pgsql-use_native_backend_protocol off/on) as a first-class test axis, tracking native-backend PR sysown#5882; differential harness grows to 6 targets (proxy-libpq / proxy-native / direct x text/binary). - Reframe LISTEN/NOTIFY as a per-mode contract test; NOTIFY forwarding is owned by sysown#5882 (already ships pgsql-native_notify-t), not this spec.
plisandro
pushed a commit
to plisandro/proxysql
that referenced
this pull request
Aug 10, 2026
…consistency hardening (final review) - diff.py: snapshot/restore pgsql-use_native_backend_protocol around the target loop in _run() (shared by run_case/run_case_sql) so a native-mode toggle never leaks into later cases once PR sysown#5882 lands the variable; a pure no-op today since the variable is absent. - conftest.py: pin client_encoding=UTF8 on the proxy DSN, matching targets.py and drivers/python/adapter.py. - behaviors/{connect,prepared,session_isolation}.py: wrap bodies in try/finally so connections close even on assert failure; make PsycopgAdapter.close() idempotent since session_isolation.py's finally may close an already-closed connection. - behaviors/transactions.py: fix stale comment pointing at a nonexistent harness/oracle.py; oracle_w lives in tests/test_routing_oracle.py.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.0 #5882 +/- ##
==========================================
+ Coverage 60.80% 61.84% +1.04%
==========================================
Files 638 643 +5
Lines 180671 181749 +1078
Branches 45653 46266 +613
==========================================
+ Hits 109860 112411 +2551
+ Misses 48193 47315 -878
+ Partials 22618 22023 -595
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
40 issues found across 52 files
Not reviewed (too large): lib/PgSQL_Connection.cpp (~2,345 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="deps/libscram/src/scram.c">
<violation number="1" location="deps/libscram/src/scram.c:514">
P1: When channel binding is enabled, this call writes into a buffer sized for plain SCRAM and truncates the client-first nonce. Size the result from the complete channel-bound format before calling `snprintf`.</violation>
</file>
<file name="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md">
<violation number="1" location="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md:133">
P2: The pinned `expected_c_b64` literal in test 14 is invalid: it base64-decodes to the mangled 44-byte blob `p@es-server-end-point, \0...`, not to base64 of the gs2 header + SHA-256 digest (correct value begins `cD10bHMtc2VydmVyLWVuZC1wb2ludCws...`). A developer trusting this literal — or recomputing it per the plan's fallback instruction using the incorrect 22-byte header — gets a wrong assertion that masks rather than detects the channel-binding bug. Fix the header length first (see related finding), then pin the correct literal: base64 of the 24-byte header + 32-zero digest = `cD10bHMtc2VydmVyLWVuZC1wb2ludCws` + 56 `A`s.</violation>
<violation number="2" location="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md:202">
P2: Test 15 uses `read_client_first_message` to validate the cbind path, but the vendored libscram server-side reader rejects cbind flag 'p' (`case 'p': ... "client requires SCRAM channel binding, but it is not supported"` and returns false). The plan never modifies that reader, so `parsed` is always false and the final assertion `ok(parsed && cbind_flag == 'p')` can never pass — the test is guaranteed to fail, contradicting the plan's "all 15 tests ok" expectation and the "round-trip through the independent libscram server-side verifier" claim. If the reader were ever patched to accept 'p', the `build_client_final_message(client, nullptr, server_first, nullptr, 0, 0)` call would then dereference NULL `credentials` in `calculate_client_proof` (`credentials->has_scram_keys` on a nullptr, and `credentials->passwd`), crashing the test. Test 15 needs a real server-side verifier path (or to be dropped) rather than this 'p'-rejecting parse smoke.</violation>
<violation number="3" location="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md:334">
P1: Task 3 changes the gs2 header to the 22-byte `p=tls-server-end-point,,` prefix but leaves `client_first_message_bare = strdup(result + 3)`, which strips only 3 bytes. With cbind set, the stored bare form becomes `tls-server-end-point,,n=,r=...` instead of the required `n=,r=...`. Both `calculate_client_proof` and `verify_server_signature` fold `client_first_message_bare` into the AuthMessage HMAC, while the server derives its bare form by stripping the full gs2 header after parsing the client-first. The resulting proof/signature inputs won't match, so SCRAM-SHA-256-PLUS native auth would fail at runtime. None of the planned tests catch it because test 14 overrides bare manually. Fix Task 3 to also strip the cbind header length when cbind is set.</violation>
<violation number="4" location="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md:526">
P2: Tests 8 and 9 build a dummy SSL with `SSL_CTX_use_certificate` + `SSL_new` and expect `pg_tls_server_end_point` (via `SSL_get_peer_certificate`) to return the cert digest. `SSL_get_peer_certificate` returns the peer's certificate only after a completed TLS handshake; a client SSL that never handshakes has no peer cert, so it returns NULL and `pg_tls_server_end_point` returns -1. Both tests would fail on `rc >= 0` and cannot validate the digest logic against a fake, non-handshaked SSL. The digest helper should be tested against the X509 directly (or behind a real handshake fixture), not through SSL_get_peer_certificate on a dummy SSL.</violation>
<violation number="5" location="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md:787">
P1: The gs2 channel-binding header "p=tls-server-end-point,," is 24 bytes, not 22. The plan's `PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22` and all 22-based sizing (cbind-input 54 instead of 56 bytes for SHA-256, Task 8's `cbind_input[86]` claimed sufficient for SHA-512's 24+64=88) are wrong, and Task 3's own test 13 already asserts the correct 24-byte length. Following the 22-byte constant drops the trailing ",," from cbind-input, producing an invalid SCRAM-SHA-256-PLUS that Postgres rejects. Use 24 for the header length and update every dependent size (54→56, 86→88, buffer capacity comments).</violation>
</file>
<file name="lib/PgSQL_Backend_Auth.cpp">
<violation number="1" location="lib/PgSQL_Backend_Auth.cpp:125">
P1: When a TLS backend advertises SCRAM-SHA-256-PLUS, `pg_scram_client_first` always returns `nullptr` despite the cbind input already being configured. Honor the configured cbind state for the `true` case so native SCRAM-PLUS can send its `p=tls-server-end-point,,` client-first message.</violation>
<violation number="2" location="lib/PgSQL_Backend_Auth.cpp:125">
P1: When a backend offers SCRAM-SHA-256-PLUS over TLS, `use_scram_plus` is true and `pg_scram_client_first(native_scram, true)` returns nullptr from the `if (channel_binding) return nullptr;` guard, so the native connection fails with 'SCRAM client-first failed' instead of completing channel-bound auth. libscram's `build_client_first_message` already emits the `p=tls-server-end-point,,` header based on the cbind input installed by `pg_scram_set_cbind`, so the `channel_binding` bool is redundant and the early return is harmful. Remove the guard and drive the header from the cbind state.</violation>
<violation number="3" location="lib/PgSQL_Backend_Auth.cpp:157">
P2: When a backend password is 2047 bytes or longer, this copy truncates it silently before SCRAM derives the proof. Preserve the complete password for SCRAM, or detect this case and route the connection through the libpq fallback instead of attempting authentication with a different secret.</violation>
</file>
<file name="include/PgSQL_Backend_Protocol.h">
<violation number="1" location="include/PgSQL_Backend_Protocol.h:106">
P1: When a TLS backend offers `SCRAM-SHA-256-PLUS`, this API contract makes native authentication fail because `native_drive_auth` calls it with `channel_binding=true` and treats `nullptr` as an authentication failure. Implement the channel-bound client-first path before selecting `-PLUS`, or stop selecting `-PLUS` and fall back before invoking this API.</violation>
</file>
<file name="include/PgSQL_PreparedStatement.h">
<violation number="1" location="include/PgSQL_PreparedStatement.h:35">
P2: After a Describe cache is populated, prepared-statement memory usage under-reports the cache's payload allocations. Add the cached string capacities to `total_mem_usage` or otherwise include them in `get_memory_usage()` so the admin metric remains accurate.</violation>
<violation number="2" location="include/PgSQL_PreparedStatement.h:88">
P1: When a client changes `search_path` or uses a schema/temp object that changes name resolution, this global set-once cache can return the previous session's `RowDescription` without contacting PostgreSQL. The documented DDL-staleness trade-off does not cover these session-state changes; include the relevant state in the cache identity or invalidate/bypass the cache whenever it changes.</violation>
</file>
<file name="lib/PgSQL_Backend_Protocol.cpp">
<violation number="1" location="lib/PgSQL_Backend_Protocol.cpp:13">
P1: When receive boundaries repeatedly leave a partial trailing backend frame, `PgSQL_Backend_Msg_Framer` retains consumed prefixes and grows based on total received bytes, not buffered bytes. Compact `buf + pos` before reallocating so long-running streamed results cannot consume unbounded memory despite each message staying below `PGSQL_MAX_BACKEND_MSG_LEN`.</violation>
</file>
<file name="docs/superpowers/specs/2026-06-14-pgsql-native-scram-plus-design.md">
<violation number="1" location="docs/superpowers/specs/2026-06-14-pgsql-native-scram-plus-design.md:62">
P1: The buffer sizes and unit-test expectations derived from the wrong 22-byte prefix must also change: §3.2 says out_cap >= "22 + 64 = 86" and "22 + 32 = 54", and §3.9 allocates `unsigned char cbind_input[86]`. With the correct 24-byte prefix, SHA-512-signed certs need 24+64=88 bytes, so the 86-byte buffer is too small: `pg_scram_build_cbind_input_tls_server_end_point` returns -1 and the code hits `assert(0)`/teardown, so a valid -PLUS connection with a SHA-512-signed backend cert cannot complete. Tests 11/12 (54/86-byte buffers) and Test 13's `22+digest_len` are likewise off by two.</violation>
<violation number="2" location="docs/superpowers/specs/2026-06-14-pgsql-native-scram-plus-design.md:71">
P1: The gs2-cbind prefix `"p=tls-server-end-point,,"` is 24 bytes (p= + 20-byte `tls-server-end-point` + two commas), not 22. §3.2's `memcpy(out, "p=tls-server-end-point,,", 22)` copies only 22 of the 24 bytes, dropping the final comma, so the `c=` value in client-final becomes `p=tls-server-end-point,<digest>` (one comma short). The server computes its expected channel-binding data as base64("p=tls-server-end-point,," || digest), so this forces a c=/proof mismatch and every -PLUS attempt fails and falls back to libpq. Fix the constant to 24 and update the buffer sizing and tests that derive from it.</violation>
<violation number="3" location="docs/superpowers/specs/2026-06-14-pgsql-native-scram-plus-design.md:71">
P2: The cbind prefix length is wrong throughout this spec. `"p=tls-server-end-point,,"` is 24 bytes, not 22. The memcpy in §3.2 copies only 22 bytes of the literal (dropping the final comma), producing a malformed SCRAM channel-binding header that will fail server-side `c=`/proof verification and force a libpq fallback whenever `-PLUS` is selected. The derived buffer sizes are also wrong: `22 + 64 = 86` should be `24 + 64 = 88`, so the §6 claim that an 86-byte buffer covers the 64-byte (SHA-512) worst case is incorrect — an implementer following the doc would under-size `cbind_input[86]` (the §3.9 call site). Update every length/constant in one pass: 22→24 for the prefix, 54→56 and 86→88 for the buffer sizes and Test 11/12 pinned lengths.</violation>
</file>
<file name="common_mk/openssl_flags.mk">
<violation number="1" location="common_mk/openssl_flags.mk:43">
P2: This file is explicitly documented as a local build workaround that must not be committed. Remove the added OpenSSL-selection changes from the PR so shared builds do not inherit this machine-dependent library selection.</violation>
</file>
<file name="include/PgSQL_Connection.h">
<violation number="1" location="include/PgSQL_Connection.h:499">
P2: For PostgreSQL 10 and newer, this produces a different value from `PQserverVersion` (`16.2` becomes `160200` instead of `160002`). Preserve the pre-10 three-component encoding only for major versions below 10.</violation>
</file>
<file name="docs/superpowers/plans/2026-07-07-pgsql-native-copy-harden-extq-wiring.md">
<violation number="1" location="docs/superpowers/plans/2026-07-07-pgsql-native-copy-harden-extq-wiring.md:275">
P1: The CopyFail safety net in Task 2 Step 2 sends CopyFail via `native_send_or_buffer(PG_Native_Conn_St::DONE)` and then immediately `continue`s into the read loop. On a non-blocking partial write, the CopyFail bytes remain queued in `native_outbuf`, and the next `FRAME_NEED_MORE` sets `async_exit_status` to PG_EVENT_READ, overwriting the pending POLLOUT; the backend is still waiting for the CopyFail and will never send the ErrorResponse/ReadyForQuery the drive is reading for, so the connection hangs. The already-implemented Sync-injection recovery in `native_fetch_result_cont` handles exactly this case by returning after the send when `async_exit_status == PG_EVENT_WRITE || !native_outbuf.empty() || !native_ssl_outbuf.empty()` (lib/PgSQL_Connection.cpp:2887-2894). Mirror that guard for the CopyFail send.</violation>
</file>
<file name="lib/PgSQL_Protocol.cpp">
<violation number="1" location="lib/PgSQL_Protocol.cpp:2831">
P2: When a native simple query contains `UPDATE ...; SELECT ...`, this leaves `affected_rows` set to the UPDATE count because the later SELECT command is skipped after tuple data appears. Track command boundaries and report affected rows for the final statement, matching libpq's per-result handling.</violation>
</file>
<file name="docs/superpowers/plans/2026-06-11-pgsql-native-protocol-phase-0-1.md">
<violation number="1" location="docs/superpowers/plans/2026-06-11-pgsql-native-protocol-phase-0-1.md:318">
P2: The framer accepts any backend-supplied message length without an upper bound. next() only rejects msglen < 4; for any larger value it returns FRAME_NEED_MORE while feed() keeps reallocating the internal buffer to fit every byte the peer sends. A broken or compromised backend that declares a multi-GB length (uint32, up to ~4GB) then streams bytes makes the connection grow its buffer without bound — a memory-exhaustion/DoS vector on the backend-facing decoder. Cap msglen at a reasonable maximum and return FRAME_ERROR once it is exceeded.</violation>
<violation number="2" location="docs/superpowers/plans/2026-06-11-pgsql-native-protocol-phase-0-1.md:322">
P2: PgSQL_Backend_Msg::payload points into the framer's internal buffer, which is realloc'd on feed() and cleared once drained. Any consumer retaining m.payload across a subsequent feed()/next() reads dangling or clobbered data. The Task 1.6 post-auth handlers cache data (ParameterStatus name/value map, SCRAM server-first/server-final strings) and must copy out of the payload before more bytes are fed; the plan does not state this, so callers risk storing dangling pointers into bp's buffer. Document that returned payloads are valid only until the next feed(), and duplicate cached ParameterStatus and SCRAM inputs.</violation>
</file>
<file name="lib/PgSQL_Session.cpp">
<violation number="1" location="lib/PgSQL_Session.cpp:3666">
P2: PROCESSING_STMT_CLOSE's rc0 epilogue erases the registry entry (destroying its unique_ptr<PgSQL_Bind_Message> and releasing the shared_ptr stmt) before RequestEnd()/CurrentQuery.end() runs. handle_post_sync_close_message set extended_query_info.stmt_client_name and stmt_client_portal_name to point into that freed bind_msg / the cleared string, so they dangle for the remainder of the frame. This contradicts the A1 fix pattern documented in the same cycle boundary (clear_named_portals() must be deferred until after RequestEnd() because the event log reads those pointers). Evict the entry only after RequestEnd(), or drop the assignment of stmt_client_name/stmt_client_portal_name from freed storage.</violation>
<violation number="2" location="lib/PgSQL_Session.cpp:7442">
P2: When a named Bind returns `rc == -1`, `reset_extended_query_frame()` and `RequestEnd()` leave this active pending entry intact; only success or session reset releases it. Clear `pending_named_bind` on the error path after `RequestEnd()` so failed binds do not retain the raw packet and statement reference.</violation>
</file>
<file name="test/tap/tests/pgsql-native_notify-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_notify-t.cpp:254">
P2: The `if (!result_match && lp_nvs.size() == nt_nvs.size()) result_match = true;` override masks genuine payload/channel byte mismatches, not just the known both-zero case. When both paths receive the expected count but with different payload bytes or channel names, the payload loop sets `result_match = false`, then this override re-enables it, so the differential test reports a pass for exactly the byte-parity regression it exists to catch. The comment says the override is for when "either both 0 or both correct," but the size-only condition also passes "both wrong the same way." Restrict the override to the both-dropped-same-count case (sizes equal but different from `n_notifies`); never suppress a payload mismatch.</violation>
</file>
<file name="test/tap/tests/pgsql-native_stress-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_stress-t.cpp:155">
P2: `plan(4)` assumes exactly one record per scenario plus the coverage summary, but the S0 (and other) branches record an extra `OpRecord` on failure paths: for example when the libpq connection fails to open, S0 records "libpq conn failed" (result_match=false) and then the native branch records a second record, so S0 alone can emit two lines. Combined with S1/S2 and the summary that exceeds the planned 4 tests, producing a TAP "planned 4 but ran more" error. Make the record emission one-per-scenario regardless of the failure path (e.g. build the digest and fell_back flags first, then record once), or update the plan.</violation>
<violation number="2" location="test/tap/tests/pgsql-native_stress-t.cpp:196">
P2: In S0 both digests are built as `lp_dig += std::to_string(i) + ":ok|"` / `nt_dig += ... ":ok|"` with no dependence on the PREPARE/EXECUTE/DEALLOCATE results (each `PQexec` return is discarded via `PQclear`). The digests are therefore structurally identical, so `result_match = (lp_dig == nt_dig)` is always true and the S0 `ok()` assertion can never fail. Worse, `if (!lp)` / `if (!nt)` only test the raw PGconn pointer, not `PQstatus(...) != CONNECTION_OK` (the sibling pgsql-native_auth_differential-t.cpp checks both), so a native connection that fails to authenticate still yields a full ":ok|" digest and is reported as full native parity and coverage. This makes the S0 case give false assurance exactly where the PR claims it verifies the prepared-statement cycle. Check the connection status and make the digest reflect each statement's result so a native failure actually breaks `result_match`.</violation>
</file>
<file name="test/tap/tests/pgsql-native_prepared-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_prepared-t.cpp:390">
P2: The `ExtQCase` fields `expect_error`, `expect_sqlstate`, `describe_after_bind`, and `close_portal` are set by the P10–P20 cases but never read anywhere: `run_extq_cycle()`/`run_extq()` only consume `stmt_name`, `query`, `param_types`, `bind_steps`, and `close_stmt`. The documented "assert exact SQLSTATE on error" (e.g. P16=42601, P17=22012) never runs, so the error-path cases are guarded only by libpq-vs-native byte equality, which cannot catch both sides reporting the same wrong SQLSTATE — unlike the midframe case, which does assert `sqlstate=42601` explicitly. Either enforce `expect_sqlstate` in `run_extq_cycle()` or drop the misleading fields and case args.</violation>
</file>
<file name="lib/PgSQL_HostGroups_Manager.cpp">
<violation number="1" location="lib/PgSQL_HostGroups_Manager.cpp:3083">
P2: Native free-connection stats expose the raw ReadyForQuery byte (`I`/`T`/`E`), unlike the existing descriptive `transaction_status` values. Use `get_pg_transaction_status_str()` so native and libpq stats preserve the same output contract.</violation>
</file>
<file name="test/tap/tests/pgsql-native_cancel-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_cancel-t.cpp:176">
P2: `drainLogToNow()` does not advance the log stream, so the phase isolation it is meant to provide never happens. It calls `get_matching_lines(f_proxysql_log, "__no_such_marker_line__")`, but `get_matching_lines` (tap/utils.cpp) reads to EOF and then, because this regex never matches, executes `f_stream.seekg(init_pos)` — rewinding to the position at the start of the call. Net effect: the get-pointer is unchanged. Consequently `scanNativePhaseLog()` in the native phase starts at the offset set by `open_file_and_seek_end` and scans this test's own libpq-phase logs in addition to the native phase, so the tripwire/positive-evidence scan is not restricted to the native phase as the comments here and the combined-scan reasoning claim. Rewrite `drainLogToNow()` to read and discard lines in a forward loop (leaving the pointer at EOF) instead of calling `get_matching_lines`; otherwise the fallback/`Canceled query (native)` checks can observe stale pre-native-phase content.</violation>
</file>
<file name="test/tap/tests/pgsql-native_transactions-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_transactions-t.cpp:17">
P2: This test is registered to run in CI group `legacy-g1` (test/tap/groups/groups.json line 188), but the file's own header comment states that T1/T3/T5/T6/T7/T11/T13/T14 "report a real divergence and emit 'not ok'" because the native `PgSQL_ExplicitTxnStateMgr` is not kept in sync. Each not-ok is asserted via `cov.emit_tap()` `ok(r.result_match, ...)`, so a single divergent case fails the whole test and thus the legacy-g1 suite. This contradicts the PR's claim that legacy-g1 is green. Either the txn-tracking bugs are still present (the test will fail CI on every run) or they were fixed and these comments/assertions are stale. Resolve which is true: fix the native path so all cases pass, or handle the known-failing cases (e.g. xfail/skip) before landing, and remove the stale Known-Issues notes if they no longer apply.</violation>
</file>
<file name="test/tap/tests/pgsql-native_auth_differential-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_auth_differential-t.cpp:331">
P2: The first regex alternative and the header claim a query-path fallback message "native_mode requested but unimplemented at this stage; falling back to libpq" emitted by PgSQL_Connection::query_cont/fetch_result_cont. That string does not exist anywhere in the current tree — the only fallback log line is "native backend auth capability gap (%s) ... falling back to libpq" at lib/PgSQL_Connection.cpp:1482 (native_capability_gap). Since the native query path is now fully wired in this PR and logs no fallback, the "used native path" assertion would silently pass even if a query-path fallback were reintroduced with a different (or no) message. Drop the dead alternative, or make the capability-gap check the sole signal, and correct the header so the assertion's guarantee matches reality.</violation>
</file>
<file name="test/tap/tests/pgsql-native_streaming-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_streaming-t.cpp:90">
P2: This line allocates an EVP_MD_CTX with EVP_MD_CTX_new() only to evaluate a always-true ternary that yields "", and never frees that context — a leak and a no-op. col_hashes is fully overwritten later in the finalize loop, so the whole statement is dead. Remove it and initialize the vector directly (e.g. `fp.col_hashes.assign(fp.ncols, "");`).</violation>
</file>
<file name="lib/PgSQL_Logger.cpp">
<violation number="1" location="lib/PgSQL_Logger.cpp:1039">
P2: When a named-portal Close is logged, this new case derives `query_digest` from parser state that the Close processing does not populate. The event can therefore carry a previous or zero digest; set the Close digest explicitly, typically to zero, before constructing `PgSQL_Event`.</violation>
</file>
<file name="docs/superpowers/plans/2026-06-14-pgsql-native-txn-copy-prepared-pr1.md">
<violation number="1" location="docs/superpowers/plans/2026-06-14-pgsql-native-txn-copy-prepared-pr1.md:396">
P2: `run_case` returns early (without calling `cov.record`) when the libpq control or native connection fails to open. `main` still expects 15 case records plus the summary (plan(16)), so each early-returned case silently shrinks the ok-count and the TAP run fails with a "planned 16 but ran N" mismatch, plus the failure is un-attributed. Record a failing OpRecord (result_match=false, native_path_used=false, detail=connect error) before every early return so the plan count stays stable and diagnostics point at the failed case.</violation>
<violation number="2" location="docs/superpowers/plans/2026-06-14-pgsql-native-txn-copy-prepared-pr1.md:1195">
P2: Task 4's extended-query runner feeds `PQsendPrepare` a `const char* paramTypes[16]` filled with parameter-type *names* as C strings, but libpq's `PQsendPrepare(PGconn*, const char*, const char*, int, const Oid*)` takes a `const Oid*` array of numeric type OIDs. This won't compile, and even if coerced, type-name strings are not OIDs (see lib/PgSQL_Connection.cpp:3543 which passes `parse_param_types.data()` where `Parse_Param_Types` is a vector of Oid). Convert the parameter types to `Oid` values (with text/binary awareness via `PQexecParams`-style `uint`/`Oid` array) before calling PQsendPrepare.</violation>
</file>
<file name="test/tap/tests/unit/Makefile">
<violation number="1" location="test/tap/tests/unit/Makefile:406">
P2: `pgsql_backend_extq-t` and `pgsql_stmt_meta_cache-t` are added to `UNIT_TESTS` here, but only `pgsql_backend_auth-t` and `pgsql_backend_framing-t` were registered in `groups.json` under `unit-tests-g1`. Since `run-tests-isolated.bash` discovers a group's tests from `groups.json`, the `unit-tests-g1` TAP job will build but never run these two tests (only the ASAN-coverage workflow picks them up by listing the directory). The broken-extended-query and Describe-cache coverage this PR claims would silently be absent from the unit-tests-g1 run.</violation>
</file>
<file name="lib/PgSQL_PreparedStatement.cpp">
<violation number="1" location="lib/PgSQL_PreparedStatement.cpp:110">
P3: After a Describe cache is published, prepared-statement metadata memory statistics underreport the cache object and its payloads. Account for the cache allocation and stored payload sizes in the metadata-memory calculation.</violation>
</file>
<file name="test/tap/tests/pg_lite_client.cpp">
<violation number="1" location="test/tap/tests/pg_lite_client.cpp:330">
P3: The SCRAM guard added here is dead code and its comment is misleading. `scram` (a ProxySQL `PgSQL_Scram_State*`) is initialized to nullptr and never assigned anywhere in `handleAuthentication`, and `doSASLAuth` creates its own unrelated libscram state (`ScramState* st = scram_state_init()`), which it already frees manually on every exit path. So the guard's `~ScramGuard()` body `if (*s) pg_scram_free(*s)` never executes (`*s` is always nullptr) and provides no RAII leak protection, despite the comment claiming the "SCRAM state ... RAII-freed on every exit path." The block also references `pg_scram_free` (a libproxysql.a symbol) in a test client whose own include comment says tests sharing this file should not pull the `pg_scram_*` symbols, and `PG_LITE_CLIENT_SCRAM` is never defined anywhere in the build tree, so the block is never even compiled. Remove the whole `#ifdef PG_LITE_CLIENT_SCRAM ... #endif` block (and, if desired, the now-unused `PgSQL_Backend_Protocol.h` include).</violation>
</file>
<file name="docs/superpowers/specs/2026-06-11-pgsql-native-protocol-design.md">
<violation number="1" location="docs/superpowers/specs/2026-06-11-pgsql-native-protocol-design.md:218">
P3: §7's differential-test corpus lists "multi-round SCRAM (with and without channel binding)" as a test case, but the entire design defers channel binding: §2 defers SCRAM-SHA-256-PLUS, §4 selects plain SCRAM-SHA-256 whenever the plain mechanism is offered and routes -PLUS-only servers to the libpq fallback at connect time. Because the native path never performs channel binding, a "with channel binding" case exercises only the libpq branch and cannot be differentially compared against the native path. Drop "with and without channel binding" from the corpus, or reword it to reflect that -PLUS-only servers only exercise the fallback.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| * type. The PostgreSQL convention is an empty SCRAM username (the | ||
| * real username travels in the StartupMessage), so the header is | ||
| * "p=tls-server-end-point,,". */ | ||
| snprintf(result, len, "p=tls-server-end-point,,n=,r=%s", scram_state->client_nonce); |
There was a problem hiding this comment.
P1: When channel binding is enabled, this call writes into a buffer sized for plain SCRAM and truncates the client-first nonce. Size the result from the complete channel-bound format before calling snprintf.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At deps/libscram/src/scram.c, line 514:
<comment>When channel binding is enabled, this call writes into a buffer sized for plain SCRAM and truncates the client-first nonce. Size the result from the complete channel-bound format before calling `snprintf`.</comment>
<file context>
@@ -503,7 +506,15 @@ char *build_client_first_message(ScramState *scram_state)
+ * type. The PostgreSQL convention is an empty SCRAM username (the
+ * real username travels in the StartupMessage), so the header is
+ * "p=tls-server-end-point,,". */
+ snprintf(result, len, "p=tls-server-end-point,,n=,r=%s", scram_state->client_nonce);
+ } else {
+ snprintf(result, len, "n,,n=,r=%s", scram_state->client_nonce);
</file context>
|
|
||
| - [ ] **Step 1: Read the existing `build_client_first_message` body (lines 481–521)** | ||
|
|
||
| The function emits `n,,n=,r=<nonce>` at line 506 (`snprintf(result, len, "n,,n=,r=%s", scram_state->client_nonce);`). It also sets `client_first_message_bare = strdup(result + 3);` at line 508 (the bare form drops the `n,,` gs2 header). |
There was a problem hiding this comment.
P1: Task 3 changes the gs2 header to the 22-byte p=tls-server-end-point,, prefix but leaves client_first_message_bare = strdup(result + 3), which strips only 3 bytes. With cbind set, the stored bare form becomes tls-server-end-point,,n=,r=... instead of the required n=,r=.... Both calculate_client_proof and verify_server_signature fold client_first_message_bare into the AuthMessage HMAC, while the server derives its bare form by stripping the full gs2 header after parsing the client-first. The resulting proof/signature inputs won't match, so SCRAM-SHA-256-PLUS native auth would fail at runtime. None of the planned tests catch it because test 14 overrides bare manually. Fix Task 3 to also strip the cbind header length when cbind is set.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md, line 334:
<comment>Task 3 changes the gs2 header to the 22-byte `p=tls-server-end-point,,` prefix but leaves `client_first_message_bare = strdup(result + 3)`, which strips only 3 bytes. With cbind set, the stored bare form becomes `tls-server-end-point,,n=,r=...` instead of the required `n=,r=...`. Both `calculate_client_proof` and `verify_server_signature` fold `client_first_message_bare` into the AuthMessage HMAC, while the server derives its bare form by stripping the full gs2 header after parsing the client-first. The resulting proof/signature inputs won't match, so SCRAM-SHA-256-PLUS native auth would fail at runtime. None of the planned tests catch it because test 14 overrides bare manually. Fix Task 3 to also strip the cbind header length when cbind is set.</comment>
<file context>
@@ -0,0 +1,1144 @@
+
+- [ ] **Step 1: Read the existing `build_client_first_message` body (lines 481–521)**
+
+The function emits `n,,n=,r=<nonce>` at line 506 (`snprintf(result, len, "n,,n=,r=%s", scram_state->client_nonce);`). It also sets `client_first_message_bare = strdup(result + 3);` at line 508 (the bare form drops the `n,,` gs2 header).
+
+- [ ] **Step 2: Change the gs2 header to honor cbind**
</file context>
|
|
||
| ```cpp | ||
| static const char* const PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT = "p=tls-server-end-point,,"; | ||
| static const size_t PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22; |
There was a problem hiding this comment.
P1: The gs2 channel-binding header "p=tls-server-end-point,," is 24 bytes, not 22. The plan's PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22 and all 22-based sizing (cbind-input 54 instead of 56 bytes for SHA-256, Task 8's cbind_input[86] claimed sufficient for SHA-512's 24+64=88) are wrong, and Task 3's own test 13 already asserts the correct 24-byte length. Following the 22-byte constant drops the trailing ",," from cbind-input, producing an invalid SCRAM-SHA-256-PLUS that Postgres rejects. Use 24 for the header length and update every dependent size (54→56, 86→88, buffer capacity comments).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md, line 787:
<comment>The gs2 channel-binding header "p=tls-server-end-point,," is 24 bytes, not 22. The plan's `PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22` and all 22-based sizing (cbind-input 54 instead of 56 bytes for SHA-256, Task 8's `cbind_input[86]` claimed sufficient for SHA-512's 24+64=88) are wrong, and Task 3's own test 13 already asserts the correct 24-byte length. Following the 22-byte constant drops the trailing ",," from cbind-input, producing an invalid SCRAM-SHA-256-PLUS that Postgres rejects. Use 24 for the header length and update every dependent size (54→56, 86→88, buffer capacity comments).</comment>
<file context>
@@ -0,0 +1,1144 @@
+
+```cpp
+static const char* const PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT = "p=tls-server-end-point,,";
+static const size_t PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22;
+
+int pg_scram_build_cbind_input_tls_server_end_point(
</file context>
Suggested change
| static const size_t PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22; | |
| static const size_t PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 24; // "p=tls-server-end-point,," is 24 bytes (RFC 5802) |
| if (s == nullptr || s->st == nullptr) return nullptr; | ||
| // Channel binding ('p'/'y' gs2 flag) is a separate task; this wrapper only does | ||
| // plain SCRAM-SHA-256 with gs2 flag 'n' ("n,," header). | ||
| if (channel_binding) return nullptr; |
There was a problem hiding this comment.
P1: When a TLS backend advertises SCRAM-SHA-256-PLUS, pg_scram_client_first always returns nullptr despite the cbind input already being configured. Honor the configured cbind state for the true case so native SCRAM-PLUS can send its p=tls-server-end-point,, client-first message.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/PgSQL_Backend_Auth.cpp, line 125:
<comment>When a TLS backend advertises SCRAM-SHA-256-PLUS, `pg_scram_client_first` always returns `nullptr` despite the cbind input already being configured. Honor the configured cbind state for the `true` case so native SCRAM-PLUS can send its `p=tls-server-end-point,,` client-first message.</comment>
<file context>
@@ -0,0 +1,226 @@
+ if (s == nullptr || s->st == nullptr) return nullptr;
+ // Channel binding ('p'/'y' gs2 flag) is a separate task; this wrapper only does
+ // plain SCRAM-SHA-256 with gs2 flag 'n' ("n,," header).
+ if (channel_binding) return nullptr;
+ scram_reset_error();
+ // libscram emits "n,,n=,r=<nonce>" and stashes client_nonce / client_first_message_bare
</file context>
| // gs2 header is "n,," (no channel binding) and the username field is empty ("n="), | ||
| // matching the PostgreSQL convention where the real username travels in the startup | ||
| // packet. Returns the owned message string, or nullptr on error (see scram_error()). | ||
| // channel_binding=true is not supported by this task and returns nullptr. |
There was a problem hiding this comment.
P1: When a TLS backend offers SCRAM-SHA-256-PLUS, this API contract makes native authentication fail because native_drive_auth calls it with channel_binding=true and treats nullptr as an authentication failure. Implement the channel-bound client-first path before selecting -PLUS, or stop selecting -PLUS and fall back before invoking this API.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At include/PgSQL_Backend_Protocol.h, line 106:
<comment>When a TLS backend offers `SCRAM-SHA-256-PLUS`, this API contract makes native authentication fail because `native_drive_auth` calls it with `channel_binding=true` and treats `nullptr` as an authentication failure. Implement the channel-bound client-first path before selecting `-PLUS`, or stop selecting `-PLUS` and fall back before invoking this API.</comment>
<file context>
@@ -0,0 +1,180 @@
+// gs2 header is "n,," (no channel binding) and the username field is empty ("n="),
+// matching the PostgreSQL convention where the real username travels in the startup
+// packet. Returns the owned message string, or nullptr on error (see scram_error()).
+// channel_binding=true is not supported by this task and returns nullptr.
+const char* pg_scram_client_first(PgSQL_Scram_State* s, bool channel_binding);
+
</file context>
| UNIT_TESTS := smoke_test-t query_cache_unit-t query_processor_unit-t \ | ||
| protocol_unit-t auth_unit-t connection_pool_unit-t \ | ||
| rule_matching_unit-t hostgroups_unit-t monitor_health_unit-t \ | ||
| pgsql_backend_framing-t pgsql_backend_auth-t pgsql_backend_extq-t \ |
There was a problem hiding this comment.
P2: pgsql_backend_extq-t and pgsql_stmt_meta_cache-t are added to UNIT_TESTS here, but only pgsql_backend_auth-t and pgsql_backend_framing-t were registered in groups.json under unit-tests-g1. Since run-tests-isolated.bash discovers a group's tests from groups.json, the unit-tests-g1 TAP job will build but never run these two tests (only the ASAN-coverage workflow picks them up by listing the directory). The broken-extended-query and Describe-cache coverage this PR claims would silently be absent from the unit-tests-g1 run.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/tap/tests/unit/Makefile, line 406:
<comment>`pgsql_backend_extq-t` and `pgsql_stmt_meta_cache-t` are added to `UNIT_TESTS` here, but only `pgsql_backend_auth-t` and `pgsql_backend_framing-t` were registered in `groups.json` under `unit-tests-g1`. Since `run-tests-isolated.bash` discovers a group's tests from `groups.json`, the `unit-tests-g1` TAP job will build but never run these two tests (only the ASAN-coverage workflow picks them up by listing the directory). The broken-extended-query and Describe-cache coverage this PR claims would silently be absent from the unit-tests-g1 run.</comment>
<file context>
@@ -404,7 +403,9 @@ $(LIBPROXYSQLAR): FORCE
UNIT_TESTS := smoke_test-t query_cache_unit-t query_processor_unit-t \
protocol_unit-t auth_unit-t connection_pool_unit-t \
rule_matching_unit-t hostgroups_unit-t monitor_health_unit-t \
+ pgsql_backend_framing-t pgsql_backend_auth-t pgsql_backend_extq-t \
pgsql_command_complete_unit-t \
+ pgsql_stmt_meta_cache-t \
</file context>
| size_t total = 1 + msglen; // type byte + length-prefixed body | ||
| if (len - pos < total) return FRAME_NEED_MORE; | ||
| out.type = (char)buf[pos]; | ||
| out.payload = buf + pos + 5; |
There was a problem hiding this comment.
P2: PgSQL_Backend_Msg::payload points into the framer's internal buffer, which is realloc'd on feed() and cleared once drained. Any consumer retaining m.payload across a subsequent feed()/next() reads dangling or clobbered data. The Task 1.6 post-auth handlers cache data (ParameterStatus name/value map, SCRAM server-first/server-final strings) and must copy out of the payload before more bytes are fed; the plan does not state this, so callers risk storing dangling pointers into bp's buffer. Document that returned payloads are valid only until the next feed(), and duplicate cached ParameterStatus and SCRAM inputs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/superpowers/plans/2026-06-11-pgsql-native-protocol-phase-0-1.md, line 322:
<comment>PgSQL_Backend_Msg::payload points into the framer's internal buffer, which is realloc'd on feed() and cleared once drained. Any consumer retaining m.payload across a subsequent feed()/next() reads dangling or clobbered data. The Task 1.6 post-auth handlers cache data (ParameterStatus name/value map, SCRAM server-first/server-final strings) and must copy out of the payload before more bytes are fed; the plan does not state this, so callers risk storing dangling pointers into bp's buffer. Document that returned payloads are valid only until the next feed(), and duplicate cached ParameterStatus and SCRAM inputs.</comment>
<file context>
@@ -0,0 +1,798 @@
+ size_t total = 1 + msglen; // type byte + length-prefixed body
+ if (len - pos < total) return FRAME_NEED_MORE;
+ out.type = (char)buf[pos];
+ out.payload = buf + pos + 5;
+ out.payload_len = msglen - 4;
+ pos += total;
</file context>
| // Set-once: install only while the slot is still empty. On success the slot now | ||
| // owns `candidate`. On failure another publish already won, so free our copy — | ||
| // the caller must not touch `candidate` after this returns either way. | ||
| if (describe_cache.compare_exchange_strong(expected, candidate, |
There was a problem hiding this comment.
P3: After a Describe cache is published, prepared-statement metadata memory statistics underreport the cache object and its payloads. Account for the cache allocation and stored payload sizes in the metadata-memory calculation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/PgSQL_PreparedStatement.cpp, line 110:
<comment>After a Describe cache is published, prepared-statement metadata memory statistics underreport the cache object and its payloads. Account for the cache allocation and stored payload sizes in the metadata-memory calculation.</comment>
<file context>
@@ -98,6 +98,21 @@ PgSQL_STMT_Global_info::~PgSQL_STMT_Global_info() {
+ // Set-once: install only while the slot is still empty. On success the slot now
+ // owns `candidate`. On failure another publish already won, so free our copy —
+ // the caller must not touch `candidate` after this returns either way.
+ if (describe_cache.compare_exchange_strong(expected, candidate,
+ std::memory_order_acq_rel, std::memory_order_acquire)) {
+ return true;
</file context>
| // The SCRAM state persists across the multi-round SASL handshake (10 -> 11 -> | ||
| // 12 -> 0). RAII-freed on every exit path (throw or return) so a mid-handshake | ||
| // failure cannot leak the libscram state. | ||
| PgSQL_Scram_State* scram = nullptr; |
There was a problem hiding this comment.
P3: The SCRAM guard added here is dead code and its comment is misleading. scram (a ProxySQL PgSQL_Scram_State*) is initialized to nullptr and never assigned anywhere in handleAuthentication, and doSASLAuth creates its own unrelated libscram state (ScramState* st = scram_state_init()), which it already frees manually on every exit path. So the guard's ~ScramGuard() body if (*s) pg_scram_free(*s) never executes (*s is always nullptr) and provides no RAII leak protection, despite the comment claiming the "SCRAM state ... RAII-freed on every exit path." The block also references pg_scram_free (a libproxysql.a symbol) in a test client whose own include comment says tests sharing this file should not pull the pg_scram_* symbols, and PG_LITE_CLIENT_SCRAM is never defined anywhere in the build tree, so the block is never even compiled. Remove the whole #ifdef PG_LITE_CLIENT_SCRAM ... #endif block (and, if desired, the now-unused PgSQL_Backend_Protocol.h include).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/tap/tests/pg_lite_client.cpp, line 330:
<comment>The SCRAM guard added here is dead code and its comment is misleading. `scram` (a ProxySQL `PgSQL_Scram_State*`) is initialized to nullptr and never assigned anywhere in `handleAuthentication`, and `doSASLAuth` creates its own unrelated libscram state (`ScramState* st = scram_state_init()`), which it already frees manually on every exit path. So the guard's `~ScramGuard()` body `if (*s) pg_scram_free(*s)` never executes (`*s` is always nullptr) and provides no RAII leak protection, despite the comment claiming the "SCRAM state ... RAII-freed on every exit path." The block also references `pg_scram_free` (a libproxysql.a symbol) in a test client whose own include comment says tests sharing this file should not pull the `pg_scram_*` symbols, and `PG_LITE_CLIENT_SCRAM` is never defined anywhere in the build tree, so the block is never even compiled. Remove the whole `#ifdef PG_LITE_CLIENT_SCRAM ... #endif` block (and, if desired, the now-unused `PgSQL_Backend_Protocol.h` include).</comment>
<file context>
@@ -315,6 +323,17 @@ void PgConnection::handleAuthentication(const std::string& password) {
+ // The SCRAM state persists across the multi-round SASL handshake (10 -> 11 ->
+ // 12 -> 0). RAII-freed on every exit path (throw or return) so a mid-handshake
+ // failure cannot leak the libscram state.
+ PgSQL_Scram_State* scram = nullptr;
+ struct ScramGuard {
+ PgSQL_Scram_State** s;
</file context>
| server-version-dependent strings. | ||
| - **Corpus.** Scalar/row/empty/error results; every data type in text and binary format; | ||
| multi-statement queries; COPY in/out; `NOTIFY`; multi-round SCRAM (with and without | ||
| channel binding), md5, cleartext; TLS on/off; large result sets (multi-buffer |
There was a problem hiding this comment.
P3: §7's differential-test corpus lists "multi-round SCRAM (with and without channel binding)" as a test case, but the entire design defers channel binding: §2 defers SCRAM-SHA-256-PLUS, §4 selects plain SCRAM-SHA-256 whenever the plain mechanism is offered and routes -PLUS-only servers to the libpq fallback at connect time. Because the native path never performs channel binding, a "with channel binding" case exercises only the libpq branch and cannot be differentially compared against the native path. Drop "with and without channel binding" from the corpus, or reword it to reflect that -PLUS-only servers only exercise the fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/superpowers/specs/2026-06-11-pgsql-native-protocol-design.md, line 218:
<comment>§7's differential-test corpus lists "multi-round SCRAM (with and without channel binding)" as a test case, but the entire design defers channel binding: §2 defers SCRAM-SHA-256-PLUS, §4 selects plain SCRAM-SHA-256 whenever the plain mechanism is offered and routes -PLUS-only servers to the libpq fallback at connect time. Because the native path never performs channel binding, a "with channel binding" case exercises only the libpq branch and cannot be differentially compared against the native path. Drop "with and without channel binding" from the corpus, or reword it to reflect that -PLUS-only servers only exercise the fallback.</comment>
<file context>
@@ -0,0 +1,253 @@
+ server-version-dependent strings.
+- **Corpus.** Scalar/row/empty/error results; every data type in text and binary format;
+ multi-statement queries; COPY in/out; `NOTIFY`; multi-round SCRAM (with and without
+ channel binding), md5, cleartext; TLS on/off; large result sets (multi-buffer
+ framing); mid-session `SET client_encoding`. Error cases compare parsed `ErrorResponse`
+ fields.
</file context>
There was a problem hiding this comment.
40 issues found across 52 files
Not reviewed (too large): lib/PgSQL_Connection.cpp (~2,345 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="deps/libscram/src/scram.c">
<violation number="1" location="deps/libscram/src/scram.c:514">
P1: When channel binding is enabled, this call writes into a buffer sized for plain SCRAM and truncates the client-first nonce. Size the result from the complete channel-bound format before calling `snprintf`.</violation>
</file>
<file name="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md">
<violation number="1" location="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md:133">
P2: The pinned `expected_c_b64` literal in test 14 is invalid: it base64-decodes to the mangled 44-byte blob `p@es-server-end-point, \0...`, not to base64 of the gs2 header + SHA-256 digest (correct value begins `cD10bHMtc2VydmVyLWVuZC1wb2ludCws...`). A developer trusting this literal — or recomputing it per the plan's fallback instruction using the incorrect 22-byte header — gets a wrong assertion that masks rather than detects the channel-binding bug. Fix the header length first (see related finding), then pin the correct literal: base64 of the 24-byte header + 32-zero digest = `cD10bHMtc2VydmVyLWVuZC1wb2ludCws` + 56 `A`s.</violation>
<violation number="2" location="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md:202">
P2: Test 15 uses `read_client_first_message` to validate the cbind path, but the vendored libscram server-side reader rejects cbind flag 'p' (`case 'p': ... "client requires SCRAM channel binding, but it is not supported"` and returns false). The plan never modifies that reader, so `parsed` is always false and the final assertion `ok(parsed && cbind_flag == 'p')` can never pass — the test is guaranteed to fail, contradicting the plan's "all 15 tests ok" expectation and the "round-trip through the independent libscram server-side verifier" claim. If the reader were ever patched to accept 'p', the `build_client_final_message(client, nullptr, server_first, nullptr, 0, 0)` call would then dereference NULL `credentials` in `calculate_client_proof` (`credentials->has_scram_keys` on a nullptr, and `credentials->passwd`), crashing the test. Test 15 needs a real server-side verifier path (or to be dropped) rather than this 'p'-rejecting parse smoke.</violation>
<violation number="3" location="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md:334">
P1: Task 3 changes the gs2 header to the 22-byte `p=tls-server-end-point,,` prefix but leaves `client_first_message_bare = strdup(result + 3)`, which strips only 3 bytes. With cbind set, the stored bare form becomes `tls-server-end-point,,n=,r=...` instead of the required `n=,r=...`. Both `calculate_client_proof` and `verify_server_signature` fold `client_first_message_bare` into the AuthMessage HMAC, while the server derives its bare form by stripping the full gs2 header after parsing the client-first. The resulting proof/signature inputs won't match, so SCRAM-SHA-256-PLUS native auth would fail at runtime. None of the planned tests catch it because test 14 overrides bare manually. Fix Task 3 to also strip the cbind header length when cbind is set.</violation>
<violation number="4" location="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md:526">
P2: Tests 8 and 9 build a dummy SSL with `SSL_CTX_use_certificate` + `SSL_new` and expect `pg_tls_server_end_point` (via `SSL_get_peer_certificate`) to return the cert digest. `SSL_get_peer_certificate` returns the peer's certificate only after a completed TLS handshake; a client SSL that never handshakes has no peer cert, so it returns NULL and `pg_tls_server_end_point` returns -1. Both tests would fail on `rc >= 0` and cannot validate the digest logic against a fake, non-handshaked SSL. The digest helper should be tested against the X509 directly (or behind a real handshake fixture), not through SSL_get_peer_certificate on a dummy SSL.</violation>
<violation number="5" location="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md:787">
P1: The gs2 channel-binding header "p=tls-server-end-point,," is 24 bytes, not 22. The plan's `PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22` and all 22-based sizing (cbind-input 54 instead of 56 bytes for SHA-256, Task 8's `cbind_input[86]` claimed sufficient for SHA-512's 24+64=88) are wrong, and Task 3's own test 13 already asserts the correct 24-byte length. Following the 22-byte constant drops the trailing ",," from cbind-input, producing an invalid SCRAM-SHA-256-PLUS that Postgres rejects. Use 24 for the header length and update every dependent size (54→56, 86→88, buffer capacity comments).</violation>
</file>
<file name="lib/PgSQL_Backend_Auth.cpp">
<violation number="1" location="lib/PgSQL_Backend_Auth.cpp:125">
P1: When a TLS backend advertises SCRAM-SHA-256-PLUS, `pg_scram_client_first` always returns `nullptr` despite the cbind input already being configured. Honor the configured cbind state for the `true` case so native SCRAM-PLUS can send its `p=tls-server-end-point,,` client-first message.</violation>
<violation number="2" location="lib/PgSQL_Backend_Auth.cpp:125">
P1: When a backend offers SCRAM-SHA-256-PLUS over TLS, `use_scram_plus` is true and `pg_scram_client_first(native_scram, true)` returns nullptr from the `if (channel_binding) return nullptr;` guard, so the native connection fails with 'SCRAM client-first failed' instead of completing channel-bound auth. libscram's `build_client_first_message` already emits the `p=tls-server-end-point,,` header based on the cbind input installed by `pg_scram_set_cbind`, so the `channel_binding` bool is redundant and the early return is harmful. Remove the guard and drive the header from the cbind state.</violation>
<violation number="3" location="lib/PgSQL_Backend_Auth.cpp:157">
P2: When a backend password is 2047 bytes or longer, this copy truncates it silently before SCRAM derives the proof. Preserve the complete password for SCRAM, or detect this case and route the connection through the libpq fallback instead of attempting authentication with a different secret.</violation>
</file>
<file name="include/PgSQL_Backend_Protocol.h">
<violation number="1" location="include/PgSQL_Backend_Protocol.h:106">
P1: When a TLS backend offers `SCRAM-SHA-256-PLUS`, this API contract makes native authentication fail because `native_drive_auth` calls it with `channel_binding=true` and treats `nullptr` as an authentication failure. Implement the channel-bound client-first path before selecting `-PLUS`, or stop selecting `-PLUS` and fall back before invoking this API.</violation>
</file>
<file name="include/PgSQL_PreparedStatement.h">
<violation number="1" location="include/PgSQL_PreparedStatement.h:35">
P2: After a Describe cache is populated, prepared-statement memory usage under-reports the cache's payload allocations. Add the cached string capacities to `total_mem_usage` or otherwise include them in `get_memory_usage()` so the admin metric remains accurate.</violation>
<violation number="2" location="include/PgSQL_PreparedStatement.h:88">
P1: When a client changes `search_path` or uses a schema/temp object that changes name resolution, this global set-once cache can return the previous session's `RowDescription` without contacting PostgreSQL. The documented DDL-staleness trade-off does not cover these session-state changes; include the relevant state in the cache identity or invalidate/bypass the cache whenever it changes.</violation>
</file>
<file name="lib/PgSQL_Backend_Protocol.cpp">
<violation number="1" location="lib/PgSQL_Backend_Protocol.cpp:13">
P1: When receive boundaries repeatedly leave a partial trailing backend frame, `PgSQL_Backend_Msg_Framer` retains consumed prefixes and grows based on total received bytes, not buffered bytes. Compact `buf + pos` before reallocating so long-running streamed results cannot consume unbounded memory despite each message staying below `PGSQL_MAX_BACKEND_MSG_LEN`.</violation>
</file>
<file name="docs/superpowers/specs/2026-06-14-pgsql-native-scram-plus-design.md">
<violation number="1" location="docs/superpowers/specs/2026-06-14-pgsql-native-scram-plus-design.md:62">
P1: The buffer sizes and unit-test expectations derived from the wrong 22-byte prefix must also change: §3.2 says out_cap >= "22 + 64 = 86" and "22 + 32 = 54", and §3.9 allocates `unsigned char cbind_input[86]`. With the correct 24-byte prefix, SHA-512-signed certs need 24+64=88 bytes, so the 86-byte buffer is too small: `pg_scram_build_cbind_input_tls_server_end_point` returns -1 and the code hits `assert(0)`/teardown, so a valid -PLUS connection with a SHA-512-signed backend cert cannot complete. Tests 11/12 (54/86-byte buffers) and Test 13's `22+digest_len` are likewise off by two.</violation>
<violation number="2" location="docs/superpowers/specs/2026-06-14-pgsql-native-scram-plus-design.md:71">
P1: The gs2-cbind prefix `"p=tls-server-end-point,,"` is 24 bytes (p= + 20-byte `tls-server-end-point` + two commas), not 22. §3.2's `memcpy(out, "p=tls-server-end-point,,", 22)` copies only 22 of the 24 bytes, dropping the final comma, so the `c=` value in client-final becomes `p=tls-server-end-point,<digest>` (one comma short). The server computes its expected channel-binding data as base64("p=tls-server-end-point,," || digest), so this forces a c=/proof mismatch and every -PLUS attempt fails and falls back to libpq. Fix the constant to 24 and update the buffer sizing and tests that derive from it.</violation>
<violation number="3" location="docs/superpowers/specs/2026-06-14-pgsql-native-scram-plus-design.md:71">
P2: The cbind prefix length is wrong throughout this spec. `"p=tls-server-end-point,,"` is 24 bytes, not 22. The memcpy in §3.2 copies only 22 bytes of the literal (dropping the final comma), producing a malformed SCRAM channel-binding header that will fail server-side `c=`/proof verification and force a libpq fallback whenever `-PLUS` is selected. The derived buffer sizes are also wrong: `22 + 64 = 86` should be `24 + 64 = 88`, so the §6 claim that an 86-byte buffer covers the 64-byte (SHA-512) worst case is incorrect — an implementer following the doc would under-size `cbind_input[86]` (the §3.9 call site). Update every length/constant in one pass: 22→24 for the prefix, 54→56 and 86→88 for the buffer sizes and Test 11/12 pinned lengths.</violation>
</file>
<file name="common_mk/openssl_flags.mk">
<violation number="1" location="common_mk/openssl_flags.mk:43">
P2: This file is explicitly documented as a local build workaround that must not be committed. Remove the added OpenSSL-selection changes from the PR so shared builds do not inherit this machine-dependent library selection.</violation>
</file>
<file name="include/PgSQL_Connection.h">
<violation number="1" location="include/PgSQL_Connection.h:499">
P2: For PostgreSQL 10 and newer, this produces a different value from `PQserverVersion` (`16.2` becomes `160200` instead of `160002`). Preserve the pre-10 three-component encoding only for major versions below 10.</violation>
</file>
<file name="docs/superpowers/plans/2026-07-07-pgsql-native-copy-harden-extq-wiring.md">
<violation number="1" location="docs/superpowers/plans/2026-07-07-pgsql-native-copy-harden-extq-wiring.md:275">
P1: The CopyFail safety net in Task 2 Step 2 sends CopyFail via `native_send_or_buffer(PG_Native_Conn_St::DONE)` and then immediately `continue`s into the read loop. On a non-blocking partial write, the CopyFail bytes remain queued in `native_outbuf`, and the next `FRAME_NEED_MORE` sets `async_exit_status` to PG_EVENT_READ, overwriting the pending POLLOUT; the backend is still waiting for the CopyFail and will never send the ErrorResponse/ReadyForQuery the drive is reading for, so the connection hangs. The already-implemented Sync-injection recovery in `native_fetch_result_cont` handles exactly this case by returning after the send when `async_exit_status == PG_EVENT_WRITE || !native_outbuf.empty() || !native_ssl_outbuf.empty()` (lib/PgSQL_Connection.cpp:2887-2894). Mirror that guard for the CopyFail send.</violation>
</file>
<file name="lib/PgSQL_Protocol.cpp">
<violation number="1" location="lib/PgSQL_Protocol.cpp:2831">
P2: When a native simple query contains `UPDATE ...; SELECT ...`, this leaves `affected_rows` set to the UPDATE count because the later SELECT command is skipped after tuple data appears. Track command boundaries and report affected rows for the final statement, matching libpq's per-result handling.</violation>
</file>
<file name="docs/superpowers/plans/2026-06-11-pgsql-native-protocol-phase-0-1.md">
<violation number="1" location="docs/superpowers/plans/2026-06-11-pgsql-native-protocol-phase-0-1.md:318">
P2: The framer accepts any backend-supplied message length without an upper bound. next() only rejects msglen < 4; for any larger value it returns FRAME_NEED_MORE while feed() keeps reallocating the internal buffer to fit every byte the peer sends. A broken or compromised backend that declares a multi-GB length (uint32, up to ~4GB) then streams bytes makes the connection grow its buffer without bound — a memory-exhaustion/DoS vector on the backend-facing decoder. Cap msglen at a reasonable maximum and return FRAME_ERROR once it is exceeded.</violation>
<violation number="2" location="docs/superpowers/plans/2026-06-11-pgsql-native-protocol-phase-0-1.md:322">
P2: PgSQL_Backend_Msg::payload points into the framer's internal buffer, which is realloc'd on feed() and cleared once drained. Any consumer retaining m.payload across a subsequent feed()/next() reads dangling or clobbered data. The Task 1.6 post-auth handlers cache data (ParameterStatus name/value map, SCRAM server-first/server-final strings) and must copy out of the payload before more bytes are fed; the plan does not state this, so callers risk storing dangling pointers into bp's buffer. Document that returned payloads are valid only until the next feed(), and duplicate cached ParameterStatus and SCRAM inputs.</violation>
</file>
<file name="lib/PgSQL_Session.cpp">
<violation number="1" location="lib/PgSQL_Session.cpp:3666">
P2: PROCESSING_STMT_CLOSE's rc0 epilogue erases the registry entry (destroying its unique_ptr<PgSQL_Bind_Message> and releasing the shared_ptr stmt) before RequestEnd()/CurrentQuery.end() runs. handle_post_sync_close_message set extended_query_info.stmt_client_name and stmt_client_portal_name to point into that freed bind_msg / the cleared string, so they dangle for the remainder of the frame. This contradicts the A1 fix pattern documented in the same cycle boundary (clear_named_portals() must be deferred until after RequestEnd() because the event log reads those pointers). Evict the entry only after RequestEnd(), or drop the assignment of stmt_client_name/stmt_client_portal_name from freed storage.</violation>
<violation number="2" location="lib/PgSQL_Session.cpp:7442">
P2: When a named Bind returns `rc == -1`, `reset_extended_query_frame()` and `RequestEnd()` leave this active pending entry intact; only success or session reset releases it. Clear `pending_named_bind` on the error path after `RequestEnd()` so failed binds do not retain the raw packet and statement reference.</violation>
</file>
<file name="test/tap/tests/pgsql-native_notify-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_notify-t.cpp:254">
P2: The `if (!result_match && lp_nvs.size() == nt_nvs.size()) result_match = true;` override masks genuine payload/channel byte mismatches, not just the known both-zero case. When both paths receive the expected count but with different payload bytes or channel names, the payload loop sets `result_match = false`, then this override re-enables it, so the differential test reports a pass for exactly the byte-parity regression it exists to catch. The comment says the override is for when "either both 0 or both correct," but the size-only condition also passes "both wrong the same way." Restrict the override to the both-dropped-same-count case (sizes equal but different from `n_notifies`); never suppress a payload mismatch.</violation>
</file>
<file name="test/tap/tests/pgsql-native_stress-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_stress-t.cpp:155">
P2: `plan(4)` assumes exactly one record per scenario plus the coverage summary, but the S0 (and other) branches record an extra `OpRecord` on failure paths: for example when the libpq connection fails to open, S0 records "libpq conn failed" (result_match=false) and then the native branch records a second record, so S0 alone can emit two lines. Combined with S1/S2 and the summary that exceeds the planned 4 tests, producing a TAP "planned 4 but ran more" error. Make the record emission one-per-scenario regardless of the failure path (e.g. build the digest and fell_back flags first, then record once), or update the plan.</violation>
<violation number="2" location="test/tap/tests/pgsql-native_stress-t.cpp:196">
P2: In S0 both digests are built as `lp_dig += std::to_string(i) + ":ok|"` / `nt_dig += ... ":ok|"` with no dependence on the PREPARE/EXECUTE/DEALLOCATE results (each `PQexec` return is discarded via `PQclear`). The digests are therefore structurally identical, so `result_match = (lp_dig == nt_dig)` is always true and the S0 `ok()` assertion can never fail. Worse, `if (!lp)` / `if (!nt)` only test the raw PGconn pointer, not `PQstatus(...) != CONNECTION_OK` (the sibling pgsql-native_auth_differential-t.cpp checks both), so a native connection that fails to authenticate still yields a full ":ok|" digest and is reported as full native parity and coverage. This makes the S0 case give false assurance exactly where the PR claims it verifies the prepared-statement cycle. Check the connection status and make the digest reflect each statement's result so a native failure actually breaks `result_match`.</violation>
</file>
<file name="test/tap/tests/pgsql-native_prepared-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_prepared-t.cpp:390">
P2: The `ExtQCase` fields `expect_error`, `expect_sqlstate`, `describe_after_bind`, and `close_portal` are set by the P10–P20 cases but never read anywhere: `run_extq_cycle()`/`run_extq()` only consume `stmt_name`, `query`, `param_types`, `bind_steps`, and `close_stmt`. The documented "assert exact SQLSTATE on error" (e.g. P16=42601, P17=22012) never runs, so the error-path cases are guarded only by libpq-vs-native byte equality, which cannot catch both sides reporting the same wrong SQLSTATE — unlike the midframe case, which does assert `sqlstate=42601` explicitly. Either enforce `expect_sqlstate` in `run_extq_cycle()` or drop the misleading fields and case args.</violation>
</file>
<file name="lib/PgSQL_HostGroups_Manager.cpp">
<violation number="1" location="lib/PgSQL_HostGroups_Manager.cpp:3083">
P2: Native free-connection stats expose the raw ReadyForQuery byte (`I`/`T`/`E`), unlike the existing descriptive `transaction_status` values. Use `get_pg_transaction_status_str()` so native and libpq stats preserve the same output contract.</violation>
</file>
<file name="test/tap/tests/pgsql-native_cancel-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_cancel-t.cpp:176">
P2: `drainLogToNow()` does not advance the log stream, so the phase isolation it is meant to provide never happens. It calls `get_matching_lines(f_proxysql_log, "__no_such_marker_line__")`, but `get_matching_lines` (tap/utils.cpp) reads to EOF and then, because this regex never matches, executes `f_stream.seekg(init_pos)` — rewinding to the position at the start of the call. Net effect: the get-pointer is unchanged. Consequently `scanNativePhaseLog()` in the native phase starts at the offset set by `open_file_and_seek_end` and scans this test's own libpq-phase logs in addition to the native phase, so the tripwire/positive-evidence scan is not restricted to the native phase as the comments here and the combined-scan reasoning claim. Rewrite `drainLogToNow()` to read and discard lines in a forward loop (leaving the pointer at EOF) instead of calling `get_matching_lines`; otherwise the fallback/`Canceled query (native)` checks can observe stale pre-native-phase content.</violation>
</file>
<file name="test/tap/tests/pgsql-native_transactions-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_transactions-t.cpp:17">
P2: This test is registered to run in CI group `legacy-g1` (test/tap/groups/groups.json line 188), but the file's own header comment states that T1/T3/T5/T6/T7/T11/T13/T14 "report a real divergence and emit 'not ok'" because the native `PgSQL_ExplicitTxnStateMgr` is not kept in sync. Each not-ok is asserted via `cov.emit_tap()` `ok(r.result_match, ...)`, so a single divergent case fails the whole test and thus the legacy-g1 suite. This contradicts the PR's claim that legacy-g1 is green. Either the txn-tracking bugs are still present (the test will fail CI on every run) or they were fixed and these comments/assertions are stale. Resolve which is true: fix the native path so all cases pass, or handle the known-failing cases (e.g. xfail/skip) before landing, and remove the stale Known-Issues notes if they no longer apply.</violation>
</file>
<file name="test/tap/tests/pgsql-native_auth_differential-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_auth_differential-t.cpp:331">
P2: The first regex alternative and the header claim a query-path fallback message "native_mode requested but unimplemented at this stage; falling back to libpq" emitted by PgSQL_Connection::query_cont/fetch_result_cont. That string does not exist anywhere in the current tree — the only fallback log line is "native backend auth capability gap (%s) ... falling back to libpq" at lib/PgSQL_Connection.cpp:1482 (native_capability_gap). Since the native query path is now fully wired in this PR and logs no fallback, the "used native path" assertion would silently pass even if a query-path fallback were reintroduced with a different (or no) message. Drop the dead alternative, or make the capability-gap check the sole signal, and correct the header so the assertion's guarantee matches reality.</violation>
</file>
<file name="test/tap/tests/pgsql-native_streaming-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_streaming-t.cpp:90">
P2: This line allocates an EVP_MD_CTX with EVP_MD_CTX_new() only to evaluate a always-true ternary that yields "", and never frees that context — a leak and a no-op. col_hashes is fully overwritten later in the finalize loop, so the whole statement is dead. Remove it and initialize the vector directly (e.g. `fp.col_hashes.assign(fp.ncols, "");`).</violation>
</file>
<file name="lib/PgSQL_Logger.cpp">
<violation number="1" location="lib/PgSQL_Logger.cpp:1039">
P2: When a named-portal Close is logged, this new case derives `query_digest` from parser state that the Close processing does not populate. The event can therefore carry a previous or zero digest; set the Close digest explicitly, typically to zero, before constructing `PgSQL_Event`.</violation>
</file>
<file name="docs/superpowers/plans/2026-06-14-pgsql-native-txn-copy-prepared-pr1.md">
<violation number="1" location="docs/superpowers/plans/2026-06-14-pgsql-native-txn-copy-prepared-pr1.md:396">
P2: `run_case` returns early (without calling `cov.record`) when the libpq control or native connection fails to open. `main` still expects 15 case records plus the summary (plan(16)), so each early-returned case silently shrinks the ok-count and the TAP run fails with a "planned 16 but ran N" mismatch, plus the failure is un-attributed. Record a failing OpRecord (result_match=false, native_path_used=false, detail=connect error) before every early return so the plan count stays stable and diagnostics point at the failed case.</violation>
<violation number="2" location="docs/superpowers/plans/2026-06-14-pgsql-native-txn-copy-prepared-pr1.md:1195">
P2: Task 4's extended-query runner feeds `PQsendPrepare` a `const char* paramTypes[16]` filled with parameter-type *names* as C strings, but libpq's `PQsendPrepare(PGconn*, const char*, const char*, int, const Oid*)` takes a `const Oid*` array of numeric type OIDs. This won't compile, and even if coerced, type-name strings are not OIDs (see lib/PgSQL_Connection.cpp:3543 which passes `parse_param_types.data()` where `Parse_Param_Types` is a vector of Oid). Convert the parameter types to `Oid` values (with text/binary awareness via `PQexecParams`-style `uint`/`Oid` array) before calling PQsendPrepare.</violation>
</file>
<file name="test/tap/tests/unit/Makefile">
<violation number="1" location="test/tap/tests/unit/Makefile:406">
P2: `pgsql_backend_extq-t` and `pgsql_stmt_meta_cache-t` are added to `UNIT_TESTS` here, but only `pgsql_backend_auth-t` and `pgsql_backend_framing-t` were registered in `groups.json` under `unit-tests-g1`. Since `run-tests-isolated.bash` discovers a group's tests from `groups.json`, the `unit-tests-g1` TAP job will build but never run these two tests (only the ASAN-coverage workflow picks them up by listing the directory). The broken-extended-query and Describe-cache coverage this PR claims would silently be absent from the unit-tests-g1 run.</violation>
</file>
<file name="lib/PgSQL_PreparedStatement.cpp">
<violation number="1" location="lib/PgSQL_PreparedStatement.cpp:110">
P3: After a Describe cache is published, prepared-statement metadata memory statistics underreport the cache object and its payloads. Account for the cache allocation and stored payload sizes in the metadata-memory calculation.</violation>
</file>
<file name="test/tap/tests/pg_lite_client.cpp">
<violation number="1" location="test/tap/tests/pg_lite_client.cpp:330">
P3: The SCRAM guard added here is dead code and its comment is misleading. `scram` (a ProxySQL `PgSQL_Scram_State*`) is initialized to nullptr and never assigned anywhere in `handleAuthentication`, and `doSASLAuth` creates its own unrelated libscram state (`ScramState* st = scram_state_init()`), which it already frees manually on every exit path. So the guard's `~ScramGuard()` body `if (*s) pg_scram_free(*s)` never executes (`*s` is always nullptr) and provides no RAII leak protection, despite the comment claiming the "SCRAM state ... RAII-freed on every exit path." The block also references `pg_scram_free` (a libproxysql.a symbol) in a test client whose own include comment says tests sharing this file should not pull the `pg_scram_*` symbols, and `PG_LITE_CLIENT_SCRAM` is never defined anywhere in the build tree, so the block is never even compiled. Remove the whole `#ifdef PG_LITE_CLIENT_SCRAM ... #endif` block (and, if desired, the now-unused `PgSQL_Backend_Protocol.h` include).</violation>
</file>
<file name="docs/superpowers/specs/2026-06-11-pgsql-native-protocol-design.md">
<violation number="1" location="docs/superpowers/specs/2026-06-11-pgsql-native-protocol-design.md:218">
P3: §7's differential-test corpus lists "multi-round SCRAM (with and without channel binding)" as a test case, but the entire design defers channel binding: §2 defers SCRAM-SHA-256-PLUS, §4 selects plain SCRAM-SHA-256 whenever the plain mechanism is offered and routes -PLUS-only servers to the libpq fallback at connect time. Because the native path never performs channel binding, a "with channel binding" case exercises only the libpq branch and cannot be differentially compared against the native path. Drop "with and without channel binding" from the corpus, or reword it to reflect that -PLUS-only servers only exercise the fallback.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| * type. The PostgreSQL convention is an empty SCRAM username (the | ||
| * real username travels in the StartupMessage), so the header is | ||
| * "p=tls-server-end-point,,". */ | ||
| snprintf(result, len, "p=tls-server-end-point,,n=,r=%s", scram_state->client_nonce); |
There was a problem hiding this comment.
P1: When channel binding is enabled, this call writes into a buffer sized for plain SCRAM and truncates the client-first nonce. Size the result from the complete channel-bound format before calling snprintf.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At deps/libscram/src/scram.c, line 514:
<comment>When channel binding is enabled, this call writes into a buffer sized for plain SCRAM and truncates the client-first nonce. Size the result from the complete channel-bound format before calling `snprintf`.</comment>
<file context>
@@ -503,7 +506,15 @@ char *build_client_first_message(ScramState *scram_state)
+ * type. The PostgreSQL convention is an empty SCRAM username (the
+ * real username travels in the StartupMessage), so the header is
+ * "p=tls-server-end-point,,". */
+ snprintf(result, len, "p=tls-server-end-point,,n=,r=%s", scram_state->client_nonce);
+ } else {
+ snprintf(result, len, "n,,n=,r=%s", scram_state->client_nonce);
</file context>
|
|
||
| - [ ] **Step 1: Read the existing `build_client_first_message` body (lines 481–521)** | ||
|
|
||
| The function emits `n,,n=,r=<nonce>` at line 506 (`snprintf(result, len, "n,,n=,r=%s", scram_state->client_nonce);`). It also sets `client_first_message_bare = strdup(result + 3);` at line 508 (the bare form drops the `n,,` gs2 header). |
There was a problem hiding this comment.
P1: Task 3 changes the gs2 header to the 22-byte p=tls-server-end-point,, prefix but leaves client_first_message_bare = strdup(result + 3), which strips only 3 bytes. With cbind set, the stored bare form becomes tls-server-end-point,,n=,r=... instead of the required n=,r=.... Both calculate_client_proof and verify_server_signature fold client_first_message_bare into the AuthMessage HMAC, while the server derives its bare form by stripping the full gs2 header after parsing the client-first. The resulting proof/signature inputs won't match, so SCRAM-SHA-256-PLUS native auth would fail at runtime. None of the planned tests catch it because test 14 overrides bare manually. Fix Task 3 to also strip the cbind header length when cbind is set.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md, line 334:
<comment>Task 3 changes the gs2 header to the 22-byte `p=tls-server-end-point,,` prefix but leaves `client_first_message_bare = strdup(result + 3)`, which strips only 3 bytes. With cbind set, the stored bare form becomes `tls-server-end-point,,n=,r=...` instead of the required `n=,r=...`. Both `calculate_client_proof` and `verify_server_signature` fold `client_first_message_bare` into the AuthMessage HMAC, while the server derives its bare form by stripping the full gs2 header after parsing the client-first. The resulting proof/signature inputs won't match, so SCRAM-SHA-256-PLUS native auth would fail at runtime. None of the planned tests catch it because test 14 overrides bare manually. Fix Task 3 to also strip the cbind header length when cbind is set.</comment>
<file context>
@@ -0,0 +1,1144 @@
+
+- [ ] **Step 1: Read the existing `build_client_first_message` body (lines 481–521)**
+
+The function emits `n,,n=,r=<nonce>` at line 506 (`snprintf(result, len, "n,,n=,r=%s", scram_state->client_nonce);`). It also sets `client_first_message_bare = strdup(result + 3);` at line 508 (the bare form drops the `n,,` gs2 header).
+
+- [ ] **Step 2: Change the gs2 header to honor cbind**
</file context>
|
|
||
| ```cpp | ||
| static const char* const PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT = "p=tls-server-end-point,,"; | ||
| static const size_t PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22; |
There was a problem hiding this comment.
P1: The gs2 channel-binding header "p=tls-server-end-point,," is 24 bytes, not 22. The plan's PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22 and all 22-based sizing (cbind-input 54 instead of 56 bytes for SHA-256, Task 8's cbind_input[86] claimed sufficient for SHA-512's 24+64=88) are wrong, and Task 3's own test 13 already asserts the correct 24-byte length. Following the 22-byte constant drops the trailing ",," from cbind-input, producing an invalid SCRAM-SHA-256-PLUS that Postgres rejects. Use 24 for the header length and update every dependent size (54→56, 86→88, buffer capacity comments).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md, line 787:
<comment>The gs2 channel-binding header "p=tls-server-end-point,," is 24 bytes, not 22. The plan's `PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22` and all 22-based sizing (cbind-input 54 instead of 56 bytes for SHA-256, Task 8's `cbind_input[86]` claimed sufficient for SHA-512's 24+64=88) are wrong, and Task 3's own test 13 already asserts the correct 24-byte length. Following the 22-byte constant drops the trailing ",," from cbind-input, producing an invalid SCRAM-SHA-256-PLUS that Postgres rejects. Use 24 for the header length and update every dependent size (54→56, 86→88, buffer capacity comments).</comment>
<file context>
@@ -0,0 +1,1144 @@
+
+```cpp
+static const char* const PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT = "p=tls-server-end-point,,";
+static const size_t PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22;
+
+int pg_scram_build_cbind_input_tls_server_end_point(
</file context>
Suggested change
| static const size_t PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22; | |
| static const size_t PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 24; // "p=tls-server-end-point,," is 24 bytes (RFC 5802) |
| if (s == nullptr || s->st == nullptr) return nullptr; | ||
| // Channel binding ('p'/'y' gs2 flag) is a separate task; this wrapper only does | ||
| // plain SCRAM-SHA-256 with gs2 flag 'n' ("n,," header). | ||
| if (channel_binding) return nullptr; |
There was a problem hiding this comment.
P1: When a TLS backend advertises SCRAM-SHA-256-PLUS, pg_scram_client_first always returns nullptr despite the cbind input already being configured. Honor the configured cbind state for the true case so native SCRAM-PLUS can send its p=tls-server-end-point,, client-first message.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/PgSQL_Backend_Auth.cpp, line 125:
<comment>When a TLS backend advertises SCRAM-SHA-256-PLUS, `pg_scram_client_first` always returns `nullptr` despite the cbind input already being configured. Honor the configured cbind state for the `true` case so native SCRAM-PLUS can send its `p=tls-server-end-point,,` client-first message.</comment>
<file context>
@@ -0,0 +1,226 @@
+ if (s == nullptr || s->st == nullptr) return nullptr;
+ // Channel binding ('p'/'y' gs2 flag) is a separate task; this wrapper only does
+ // plain SCRAM-SHA-256 with gs2 flag 'n' ("n,," header).
+ if (channel_binding) return nullptr;
+ scram_reset_error();
+ // libscram emits "n,,n=,r=<nonce>" and stashes client_nonce / client_first_message_bare
</file context>
| // gs2 header is "n,," (no channel binding) and the username field is empty ("n="), | ||
| // matching the PostgreSQL convention where the real username travels in the startup | ||
| // packet. Returns the owned message string, or nullptr on error (see scram_error()). | ||
| // channel_binding=true is not supported by this task and returns nullptr. |
There was a problem hiding this comment.
P1: When a TLS backend offers SCRAM-SHA-256-PLUS, this API contract makes native authentication fail because native_drive_auth calls it with channel_binding=true and treats nullptr as an authentication failure. Implement the channel-bound client-first path before selecting -PLUS, or stop selecting -PLUS and fall back before invoking this API.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At include/PgSQL_Backend_Protocol.h, line 106:
<comment>When a TLS backend offers `SCRAM-SHA-256-PLUS`, this API contract makes native authentication fail because `native_drive_auth` calls it with `channel_binding=true` and treats `nullptr` as an authentication failure. Implement the channel-bound client-first path before selecting `-PLUS`, or stop selecting `-PLUS` and fall back before invoking this API.</comment>
<file context>
@@ -0,0 +1,180 @@
+// gs2 header is "n,," (no channel binding) and the username field is empty ("n="),
+// matching the PostgreSQL convention where the real username travels in the startup
+// packet. Returns the owned message string, or nullptr on error (see scram_error()).
+// channel_binding=true is not supported by this task and returns nullptr.
+const char* pg_scram_client_first(PgSQL_Scram_State* s, bool channel_binding);
+
</file context>
| UNIT_TESTS := smoke_test-t query_cache_unit-t query_processor_unit-t \ | ||
| protocol_unit-t auth_unit-t connection_pool_unit-t \ | ||
| rule_matching_unit-t hostgroups_unit-t monitor_health_unit-t \ | ||
| pgsql_backend_framing-t pgsql_backend_auth-t pgsql_backend_extq-t \ |
There was a problem hiding this comment.
P2: pgsql_backend_extq-t and pgsql_stmt_meta_cache-t are added to UNIT_TESTS here, but only pgsql_backend_auth-t and pgsql_backend_framing-t were registered in groups.json under unit-tests-g1. Since run-tests-isolated.bash discovers a group's tests from groups.json, the unit-tests-g1 TAP job will build but never run these two tests (only the ASAN-coverage workflow picks them up by listing the directory). The broken-extended-query and Describe-cache coverage this PR claims would silently be absent from the unit-tests-g1 run.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/tap/tests/unit/Makefile, line 406:
<comment>`pgsql_backend_extq-t` and `pgsql_stmt_meta_cache-t` are added to `UNIT_TESTS` here, but only `pgsql_backend_auth-t` and `pgsql_backend_framing-t` were registered in `groups.json` under `unit-tests-g1`. Since `run-tests-isolated.bash` discovers a group's tests from `groups.json`, the `unit-tests-g1` TAP job will build but never run these two tests (only the ASAN-coverage workflow picks them up by listing the directory). The broken-extended-query and Describe-cache coverage this PR claims would silently be absent from the unit-tests-g1 run.</comment>
<file context>
@@ -404,7 +403,9 @@ $(LIBPROXYSQLAR): FORCE
UNIT_TESTS := smoke_test-t query_cache_unit-t query_processor_unit-t \
protocol_unit-t auth_unit-t connection_pool_unit-t \
rule_matching_unit-t hostgroups_unit-t monitor_health_unit-t \
+ pgsql_backend_framing-t pgsql_backend_auth-t pgsql_backend_extq-t \
pgsql_command_complete_unit-t \
+ pgsql_stmt_meta_cache-t \
</file context>
| size_t total = 1 + msglen; // type byte + length-prefixed body | ||
| if (len - pos < total) return FRAME_NEED_MORE; | ||
| out.type = (char)buf[pos]; | ||
| out.payload = buf + pos + 5; |
There was a problem hiding this comment.
P2: PgSQL_Backend_Msg::payload points into the framer's internal buffer, which is realloc'd on feed() and cleared once drained. Any consumer retaining m.payload across a subsequent feed()/next() reads dangling or clobbered data. The Task 1.6 post-auth handlers cache data (ParameterStatus name/value map, SCRAM server-first/server-final strings) and must copy out of the payload before more bytes are fed; the plan does not state this, so callers risk storing dangling pointers into bp's buffer. Document that returned payloads are valid only until the next feed(), and duplicate cached ParameterStatus and SCRAM inputs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/superpowers/plans/2026-06-11-pgsql-native-protocol-phase-0-1.md, line 322:
<comment>PgSQL_Backend_Msg::payload points into the framer's internal buffer, which is realloc'd on feed() and cleared once drained. Any consumer retaining m.payload across a subsequent feed()/next() reads dangling or clobbered data. The Task 1.6 post-auth handlers cache data (ParameterStatus name/value map, SCRAM server-first/server-final strings) and must copy out of the payload before more bytes are fed; the plan does not state this, so callers risk storing dangling pointers into bp's buffer. Document that returned payloads are valid only until the next feed(), and duplicate cached ParameterStatus and SCRAM inputs.</comment>
<file context>
@@ -0,0 +1,798 @@
+ size_t total = 1 + msglen; // type byte + length-prefixed body
+ if (len - pos < total) return FRAME_NEED_MORE;
+ out.type = (char)buf[pos];
+ out.payload = buf + pos + 5;
+ out.payload_len = msglen - 4;
+ pos += total;
</file context>
| // Set-once: install only while the slot is still empty. On success the slot now | ||
| // owns `candidate`. On failure another publish already won, so free our copy — | ||
| // the caller must not touch `candidate` after this returns either way. | ||
| if (describe_cache.compare_exchange_strong(expected, candidate, |
There was a problem hiding this comment.
P3: After a Describe cache is published, prepared-statement metadata memory statistics underreport the cache object and its payloads. Account for the cache allocation and stored payload sizes in the metadata-memory calculation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/PgSQL_PreparedStatement.cpp, line 110:
<comment>After a Describe cache is published, prepared-statement metadata memory statistics underreport the cache object and its payloads. Account for the cache allocation and stored payload sizes in the metadata-memory calculation.</comment>
<file context>
@@ -98,6 +98,21 @@ PgSQL_STMT_Global_info::~PgSQL_STMT_Global_info() {
+ // Set-once: install only while the slot is still empty. On success the slot now
+ // owns `candidate`. On failure another publish already won, so free our copy —
+ // the caller must not touch `candidate` after this returns either way.
+ if (describe_cache.compare_exchange_strong(expected, candidate,
+ std::memory_order_acq_rel, std::memory_order_acquire)) {
+ return true;
</file context>
| // The SCRAM state persists across the multi-round SASL handshake (10 -> 11 -> | ||
| // 12 -> 0). RAII-freed on every exit path (throw or return) so a mid-handshake | ||
| // failure cannot leak the libscram state. | ||
| PgSQL_Scram_State* scram = nullptr; |
There was a problem hiding this comment.
P3: The SCRAM guard added here is dead code and its comment is misleading. scram (a ProxySQL PgSQL_Scram_State*) is initialized to nullptr and never assigned anywhere in handleAuthentication, and doSASLAuth creates its own unrelated libscram state (ScramState* st = scram_state_init()), which it already frees manually on every exit path. So the guard's ~ScramGuard() body if (*s) pg_scram_free(*s) never executes (*s is always nullptr) and provides no RAII leak protection, despite the comment claiming the "SCRAM state ... RAII-freed on every exit path." The block also references pg_scram_free (a libproxysql.a symbol) in a test client whose own include comment says tests sharing this file should not pull the pg_scram_* symbols, and PG_LITE_CLIENT_SCRAM is never defined anywhere in the build tree, so the block is never even compiled. Remove the whole #ifdef PG_LITE_CLIENT_SCRAM ... #endif block (and, if desired, the now-unused PgSQL_Backend_Protocol.h include).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/tap/tests/pg_lite_client.cpp, line 330:
<comment>The SCRAM guard added here is dead code and its comment is misleading. `scram` (a ProxySQL `PgSQL_Scram_State*`) is initialized to nullptr and never assigned anywhere in `handleAuthentication`, and `doSASLAuth` creates its own unrelated libscram state (`ScramState* st = scram_state_init()`), which it already frees manually on every exit path. So the guard's `~ScramGuard()` body `if (*s) pg_scram_free(*s)` never executes (`*s` is always nullptr) and provides no RAII leak protection, despite the comment claiming the "SCRAM state ... RAII-freed on every exit path." The block also references `pg_scram_free` (a libproxysql.a symbol) in a test client whose own include comment says tests sharing this file should not pull the `pg_scram_*` symbols, and `PG_LITE_CLIENT_SCRAM` is never defined anywhere in the build tree, so the block is never even compiled. Remove the whole `#ifdef PG_LITE_CLIENT_SCRAM ... #endif` block (and, if desired, the now-unused `PgSQL_Backend_Protocol.h` include).</comment>
<file context>
@@ -315,6 +323,17 @@ void PgConnection::handleAuthentication(const std::string& password) {
+ // The SCRAM state persists across the multi-round SASL handshake (10 -> 11 ->
+ // 12 -> 0). RAII-freed on every exit path (throw or return) so a mid-handshake
+ // failure cannot leak the libscram state.
+ PgSQL_Scram_State* scram = nullptr;
+ struct ScramGuard {
+ PgSQL_Scram_State** s;
</file context>
| server-version-dependent strings. | ||
| - **Corpus.** Scalar/row/empty/error results; every data type in text and binary format; | ||
| multi-statement queries; COPY in/out; `NOTIFY`; multi-round SCRAM (with and without | ||
| channel binding), md5, cleartext; TLS on/off; large result sets (multi-buffer |
There was a problem hiding this comment.
P3: §7's differential-test corpus lists "multi-round SCRAM (with and without channel binding)" as a test case, but the entire design defers channel binding: §2 defers SCRAM-SHA-256-PLUS, §4 selects plain SCRAM-SHA-256 whenever the plain mechanism is offered and routes -PLUS-only servers to the libpq fallback at connect time. Because the native path never performs channel binding, a "with channel binding" case exercises only the libpq branch and cannot be differentially compared against the native path. Drop "with and without channel binding" from the corpus, or reword it to reflect that -PLUS-only servers only exercise the fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/superpowers/specs/2026-06-11-pgsql-native-protocol-design.md, line 218:
<comment>§7's differential-test corpus lists "multi-round SCRAM (with and without channel binding)" as a test case, but the entire design defers channel binding: §2 defers SCRAM-SHA-256-PLUS, §4 selects plain SCRAM-SHA-256 whenever the plain mechanism is offered and routes -PLUS-only servers to the libpq fallback at connect time. Because the native path never performs channel binding, a "with channel binding" case exercises only the libpq branch and cannot be differentially compared against the native path. Drop "with and without channel binding" from the corpus, or reword it to reflect that -PLUS-only servers only exercise the fallback.</comment>
<file context>
@@ -0,0 +1,253 @@
+ server-version-dependent strings.
+- **Corpus.** Scalar/row/empty/error results; every data type in text and binary format;
+ multi-statement queries; COPY in/out; `NOTIFY`; multi-round SCRAM (with and without
+ channel binding), md5, cleartext; TLS on/off; large result sets (multi-buffer
+ framing); mid-session `SET client_encoding`. Error cases compare parsed `ErrorResponse`
+ fields.
</file context>
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsReplaces libpq with a native PostgreSQL backend wire-protocol implementation covering connection, authentication, COPY, simple queries, and extended queries. Consider addressing the minor audit log timing issue in the certificate authentication path where AUTH_OK is recorded before client welcome failure. 💡 Quality: AUTH_OK audit logged before welcome_client failure in cert path📄 lib/PgSQL_Session.cpp:4275-4285 📄 lib/PgSQL_Session.cpp:4236-4245 In the cert-auth branch the AUTH_OK audit entry is emitted (line ~4277) before welcome_client() is called, so when welcome_client() now returns false and the session is rejected as *wrong_pass, an AUTH_OK is still recorded for a connection that never succeeded. The other new branch (line ~4237) correctly moved log_audit_entry inside the success case. Move the log_audit_entry call into the 🤖 Prompt for agentsOptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Important Your trial ends in 3 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more. Was this helpful? React with 👍 / 👎 | Gitar |
Resolve conflicts for PR 5882: - test/tap/tests/unit/Makefile: keep vec.o single-append NOTE (both sides removed the duplicate first vec.o block; HEAD's explanatory comment kept)
A COPY switches the session to fast forward, which takes the backend's TLS session over from the connection: PgSQL_Data_Stream installs two memory BIOs on it with SSL_set_bio() so it can drive the socket itself during the relay. Nothing put libpq's own transport back. Leaving fast forward only cleared myds->encrypted and myds->ssl, so the SSL object libpq still owns kept pointing at ProxySQL's buffers. The next PQsendQuery() encrypted into a buffer nobody drains and reported success: the query never reached the backend and the session waited for a reply that could not come. The connection is returned to the pool in that state too, so a client that COPYs and disconnects strands the next session that picks it up. libpq does not use SSL_set_fd(). fe-secure-openssl.c installs a custom BIO carrying the PGconn as app data, so the displaced transport cannot be rebuilt from outside libpq and has to be saved and handed back. Adds PgSQL_Data_Stream::adopt_backend_tls() and release_backend_tls(), holding the displaced transport on PgSQL_Connection. The three copies of the handover -- attach_connection(), ASYNC_CONNECT_SUCCESSFUL and switch_normal_to_fast_forward_mode() -- now call adopt. Release runs from switch_fast_forward_to_normal_mode() and from detach_connection(), which every disposal route passes through. detach_connection() no longer tests sess->session_fast_forward. A COPY clears that flag before the connection is detached, so the condition was never true for this case. attach_connection() previously called assert(0) when the backend reported TLS in use but exposed no SSL object, aborting the process; asserts are live in release builds. The shared helper logs and marks the connection unusable instead, and does the same when a previous relay never returned the transport.
… feature/pgsql-native-backend-protocol
…into feature/pgsql-native-backend-protocol
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Replaces libpq on the ProxySQL → PostgreSQL backend data path with a native wire-protocol implementation, behind the runtime flag
pgsql-use_native_backend_protocol(default off; libpq stays compiled in as fallback and as the differential-test oracle). Monitor and plugins keep using libpq.Design specs and implementation plans are in
docs/superpowers/specs/anddocs/superpowers/plans/(2026-06-11 through 2026-07-07).Highlights
COPY ... TO STDOUTstreams natively;COPY ... FROM STDINkeeps the session fast-forward route (byte-equal, zero-copy); a CopyFail safety net turns any unexpected CopyInResponse on the native drive into a clean error instead of a protocol hang.local_stmtsclient registry, per-backend statement reuse and implicit re-Parse are all retained — only the wire layer is swapped (typed Parse/Bind/Describe/Execute/Close/Flush/Sync builders; per-step drain with ack filtering that preserves ProxySQL's BindComplete/CloseComplete/ParseComplete synthesis; pipeline-abort recovery that injects a Sync on mid-frame errors).PgSQL_STMT_Global_info(set-once, atomic publish): repeat Describes are served without a backend round trip, in both backend modes.SQL3_Free_Connections, native query errors misclassified as broken connections, a bare-ack assert crash, a CopyFail partial-send hang.Testing
Differential testing against the libpq path as oracle (a divergence is a hard failure):
pgsql-native_auth_differential,query_differential(16/16),streaming,transactions(16/16),copy(15/15, with truthful per-route coverage reporting),prepared(27/27 strict — no escape hatches; all EXT_* operations native and byte-equal, incl. named statements + DEALLOCATE, mid-frame error recovery positively asserted, cross-mode Describe-cache parity in both directions),notify,stress(200× PREPARE/SELECT/txn across the pool).legacy-g1group: all pgsql tests green; the 10 MySQL-side failures were individually root-caused as unrelated to this branch (PgSQL-only diff; mostly sharedtest.sbtest1contamination between tests — triage notes available, tracking issues to follow).Known limitations / follow-ups
-PLUS-only-server edge cases fall back to libpq at connect time (logged once per backend).https://claude.ai/code/session_015yDEBKWSWyDYq9MFxS69p7
Summary by CodeRabbit
tls-server-end-point), with graceful fallback when not available.